New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MibLoader not working in multi threaded environments #21

Closed
maspholm opened this Issue Oct 26, 2016 · 4 comments

Comments

Projects
None yet
2 participants
@maspholm
Copy link

maspholm commented Oct 26, 2016

The MibLoader.MibSource class uses a static reference to the ASN.1 parser:

private static class MibSource {
    private static Asn1Parser parser = null;

This means that two threads using the MibLoader simultaneously will use the same stateful parser object with the result that both parsings fail with unpredictive results.

Some ways to fix the issue:

  • Make the parser variable class level
  • Make the parser variable thread local
  • Make the access API synchronized to guarantee single threaded access
@cederberg

This comment has been minimized.

Copy link
Owner

cederberg commented Oct 26, 2016

There are more reasons than this for not loading MIB files in parallell from several threads. In fact, the javadoc even says so:

The MIB loader is not thread-safe, i.e. it cannot be used concurrently in multiple threads.

See https://www.mibble.org/doc/release/api/net/percederberg/mibble/MibLoader.html

I strongly recommend synchronizing all access to the MibLoader if possible.

@maspholm

This comment has been minimized.

Copy link

maspholm commented Oct 27, 2016

Being not-thread safe is one thing. Using static stateful objects so that not even two different MibLoader objects can operate in parallell is another. Using a static parser would perhaps make sense if the MibLoader was a singleton object, but it is not. It's not possible to tell from the API that you cannot even have two different MibLoader objects and use them from different threads.

The way I worked around the problem was to use a Proxy object that uses synchronization on a singleton monitor object for all load-methods. This feels like something that should be handled by the library instead of the application code.

@cederberg

This comment has been minimized.

Copy link
Owner

cederberg commented Oct 27, 2016

Ah, I see. This is a bug, separate MibLoader instances should of course be possible to use in parallell (if needed for some reason). The parser variable should obviously be an instance variable for the loader itself.

@cederberg cederberg added the Bug label Oct 27, 2016

cederberg added a commit that referenced this issue Nov 15, 2016

Fixed MibLoader to allow parallell use of multiple instances.
A static reference to the ASN1Parser instance previously inhibited
parallell usage of multiple MibLoader instances (issue #21). This
is now fixed and code is refactored for clarity. Thanks to @maspholm
for reporting this.
@cederberg

This comment has been minimized.

Copy link
Owner

cederberg commented Nov 15, 2016

This is now fixed on the master branch. Thanks for reporting this!

@cederberg cederberg closed this Nov 15, 2016

cederberg added a commit that referenced this issue Nov 15, 2016

Removed global comment token cache from MibAnalyzerUtil class.
This is related to issue #21, since the global token cache would also
prohibit multi-instance usage of the MibLoader.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment